Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for url zip subdirectories #5811

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix for url zip subdirectories #5811

wants to merge 2 commits into from
+287 −10

Conversation

ashnair1
Copy link
Contributor

@ashnair1 ashnair1 commented Jun 9, 2022 •
edited
Loading

Added support for parsing subdirectory param from zip urls

i.e. support the following

poetry add 'https://github.com/foo/bar/archive/0.1.0.zip#subdirectory=baz'
  • Added tests for changed code.

Requires: python-poetry/poetry-core#398

Original request was raised here -> #5172 (comment)

cc: @andersk @neersighted @abn

@ashnair1 ashnair1 force-pushed the zip-subdir-fix branch 3 times, most recently from 576e3ae to 4624ffb Compare June 9, 2022 08:19
@andersk
Copy link
Contributor

andersk commented Jun 10, 2022

The poetry add command succeeds, but breaks all future poetry commands when they try to parse the configuration.

$ poetry init -n

$ poetry add 'https://github.com/zulip/python-zulip-api/archive/0.8.2.zip#subdirectory=zulip'

$ poetry shell

The Poetry configuration is invalid:
  - [dependencies.zulip] {'url': 'https://github.com/zulip/python-zulip-api/archive/0.8.2.zip#subdirectory=zulip', 'subdirectory': 'zulip'} is not valid under any of the given schemas

@ashnair1 ashnair1 mentioned this pull request Jun 10, 2022
Merged
@ashnair1
Copy link
Contributor Author

The subdirectory param needs to be added to the poetry-schema in poetry-core for this to work. Opened python-poetry/poetry-core#398 to address this.

@ashnair1 ashnair1 force-pushed the zip-subdir-fix branch 2 times, most recently from a4d926b to 884c2be Compare June 14, 2022 12:30
@ashnair1
Copy link
Contributor Author

ashnair1 commented Jun 14, 2022 •
edited
Loading

This requires #5172 and python-poetry/poetry-core#398 for tests to pass. Former adds subdirectory support for locker (5e8b9ef) and the latter is required for UrlDependency with subdirectory support and schema updates.

@ashnair1
Copy link
Contributor Author

ashnair1 commented Jun 14, 2022 •
edited
Loading

Local tests

$ cat pyproject.toml 
[tool.poetry]
name = "test"
version = "0.1.0"
description = ""
authors = []

[tool.poetry.dependencies]
python="^3.9"

[build-system]
requires = ["poetry-core @ https://github.com/python-poetry/poetry-core/archive/refs/pull/398/head.zip"]
build-backend = "poetry.core.masonry.api"

$ poetry add 'https://github.com/zulip/python-zulip-api/archive/0.8.2.zip#subdirectory=zulip'
Creating virtualenv test in /media/ashwin/DATA2/poetry-test/.venv

Updating dependencies
Resolving dependencies... (15.8s)

Writing lock file

Package operations: 10 installs, 0 updates, 0 removals

  • Installing certifi (2022.5.18.1)
  • Installing charset-normalizer (2.0.12)
  • Installing idna (3.3)
  • Installing urllib3 (1.26.9)
  • Installing requests (2.28.0)
  • Installing click (8.1.3)
  • Installing distro (1.7.0)
  • Installing matrix-client (0.4.0)
  • Installing typing-extensions (4.2.0)
  • Installing zulip (0.8.2 https://github.com/zulip/python-zulip-api/archive/0.8.2.zip)

$ touch test.py

$ rm -rf /tmp/venv/

$ python -m venv /tmp/venv

$ . /tmp/venv/bin/activate

$ pip install .
Processing /media/ashwin/DATA2/poetry-test
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Collecting zulip@ https://github.com/zulip/python-zulip-api/archive/0.8.2.zip#subdirectory=zulip
  Using cached https://github.com/zulip/python-zulip-api/archive/0.8.2.zip (2.7 MB)
  Preparing metadata (setup.py) ... done
Collecting requests[security]>=0.12.1
  Using cached requests-2.28.0-py3-none-any.whl (62 kB)
Collecting matrix_client
  Using cached matrix_client-0.4.0-py2.py3-none-any.whl (43 kB)
Collecting distro
  Using cached distro-1.7.0-py3-none-any.whl (20 kB)
Collecting click
  Using cached click-8.1.3-py3-none-any.whl (96 kB)
Collecting typing_extensions>=3.7
  Using cached typing_extensions-4.2.0-py3-none-any.whl (24 kB)
Collecting urllib3<1.27,>=1.21.1
  Using cached urllib3-1.26.9-py2.py3-none-any.whl (138 kB)
Collecting charset-normalizer~=2.0.0
  Using cached charset_normalizer-2.0.12-py3-none-any.whl (39 kB)
Collecting idna<4,>=2.5
  Using cached idna-3.3-py3-none-any.whl (61 kB)
Collecting certifi>=2017.4.17
  Using cached certifi-2022.5.18.1-py3-none-any.whl (155 kB)
Using legacy 'setup.py install' for zulip, since package 'wheel' is not installed.
Building wheels for collected packages: test
  Building wheel for test (pyproject.toml) ... done
  Created wheel for test: filename=test-0.1.0-py3-none-any.whl size=975 sha256=941e48022c674808a6b6538eecc26f5ba71b212c2a5c48aad6c0b9b4ec55ecae
  Stored in directory: /home/ashwin/.cache/pip/wheels/f1/61/6f/a7f6d6a2bbb78c43b2195dbbbe1b7697a94a999f4e8d5a11a0
Successfully built test
Installing collected packages: urllib3, typing_extensions, idna, distro, click, charset-normalizer, certifi, requests, matrix_client, zulip, test
  Running setup.py install for zulip ... done
Successfully installed certifi-2022.5.18.1 charset-normalizer-2.0.12 click-8.1.3 distro-1.7.0 idna-3.3 matrix_client-0.4.0 requests-2.28.0 test-0.1.0 typing_extensions-4.2.0 urllib3-1.26.9 zulip-0.8.2
WARNING: You are using pip version 22.0.4; however, version 22.1.2 is available.
You should consider upgrading via the '/tmp/venv/bin/python -m pip install --upgrade pip' command.

@ashnair1 ashnair1 changed the title Fix for zip subdirectories Fix for url zip subdirectories Jun 15, 2022
@ashnair1 ashnair1 mentioned this pull request Jun 15, 2022
Merged
1 task
@ashnair1
Copy link
Contributor Author

Dropped link to #5172 by adding locker subdirectory support

@ashnair1 ashnair1 mentioned this pull request Jun 30, 2022
Closed
@AKuederle AKuederle mentioned this pull request Jul 4, 2022
Merged
@ashnair1
Copy link
Contributor Author

@andersk Ran some local tests above and looks like the fix works.

@neersighted
Copy link
Member

@ashnair1 Any chance you can rebase this? Might be able to sneak it into 1.2, or if not we'll scope it for the follow-up release that should hopefully be soon after.

@ashnair1
Copy link
Contributor Author

Tests should pass once python-poetry/poetry-core#398 is merged

radoering
radoering requested changes Aug 22, 2022
View reviewed changes
Copy link
Member

@radoering radoering left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry to be a killjoy, but there are still some uncertainties in this PR (plus the required core PR) so that it will probably not make it into 1.2.

However, we are planning to just release 1.3 if there are some new features (and not if one specific big feature is ready).

I just skimmed over but it seems the test coverage could be improved. Maybe, some similar tests as in #5172 can be implemented.

src/poetry/puzzle/provider.py
try:
PackageInfo._source_subdirectory = source_subdirectory
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the subdirectory as class attribute seems like a crude hack to me. I think that has to be refactored.

andersk
andersk reviewed Aug 22, 2022
View reviewed changes
src/poetry/utils/pip.py Outdated Show resolved Hide resolved
@ashnair1 ashnair1 force-pushed the zip-subdir-fix branch 2 times, most recently from c230682 to 8718ebc Compare August 27, 2022 12:20
@ashnair1 ashnair1 force-pushed the zip-subdir-fix branch 2 times, most recently from 631f8c1 to 5f8876a Compare September 3, 2022 10:55
@ashnair1 ashnair1 force-pushed the zip-subdir-fix branch 4 times, most recently from c35a1ef to e85e1a9 Compare September 19, 2022 07:56
@neersighted
Copy link
Member

Hey @ashnair1 -- we'd like to land this for 1.3 which is creeping up on us. Could you address @radoering's review so we can unstuck this?

@ashnair1
Copy link
Contributor Author

ashnair1 commented Oct 8, 2022

Hey @ashnair1 -- we'd like to land this for 1.3 which is creeping up on us. Could you address @radoering's review so we can unstuck this?

I've added some additional tests. Haven't figured out why it fails on Windows 3.8 and macOS 3.8 yet.

Regarding the refactor, it's been a while since I looked at the codebase and I'm currently a bit swamped at work, so it will take me some time to do this. If the release is indeed around the corner, I welcome someone more intimate with the codebase to take a crack at it. Otherwise, I might have some time to look into this towards the end of next week.

@neersighted neersighted mentioned this pull request Oct 9, 2022
Closed
3 tasks
@ashnair1
Copy link
Contributor Author

ashnair1 commented Oct 31, 2022 •
edited
Loading

I'm not sure why but for some tests, 'subdirectory': 'subdir' gets added when it's not supposed to. The random nature of this error might be due to a particular test being run before another that creates this whole issue. Can't seem to narrow down what's causing this though.

@radoering
Copy link
Member

That's probably a side effect of setting a class attribute. cf #5811 (comment)

@dimbleby dimbleby mentioned this pull request Apr 26, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andersk andersk andersk left review comments

@radoering radoering Awaiting requested review from radoering

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ashnair1 @andersk @neersighted @radoering